GIT-331: fix shadowing key from the kv pair #332

harshanarayana · 2022-06-18T19:07:19Z

What this PR does / why we need it:
While trying to take a stab at #275 I noticed that there is a lingering issue in the KVListFormat function in terms of how it processes the keys

klog/internal/serialize/keyvalues.go

Lines 112 to 118 in 9c48b7d

    
           if k, ok := k.(string); ok { 
        
           	// Avoid one allocation when the key is a string, which 
        
           	// normally it should be. 
        
           	b.WriteString(k) 
        
           } else { 
        
           	b.WriteString(fmt.Sprintf("%s", k)) 
        
           }

In case if the key passed to the KVListFormat is not of string type, this ends up consuming the key and only printing the value.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #331

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Non string keys are displayed as string formatted keys

pohly

/lgtm

Good find, thanks.

pohly · 2022-06-18T19:13:38Z

/assign @serathius

For approval.

dims · 2022-06-18T20:26:44Z

/approve

k8s-ci-robot · 2022-06-18T20:27:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, harshanarayana, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

GIT-331: fix shadowing key from the kv pair

e4329d2

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 18, 2022

k8s-ci-robot requested review from pohly and serathius June 18, 2022 19:07

pohly approved these changes Jun 18, 2022

View reviewed changes

k8s-ci-robot assigned pohly Jun 18, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2022

k8s-ci-robot assigned serathius Jun 18, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2022

k8s-ci-robot merged commit 49e17d5 into kubernetes:main Jun 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GIT-331: fix shadowing key from the kv pair #332

GIT-331: fix shadowing key from the kv pair #332

harshanarayana commented Jun 18, 2022

pohly left a comment

pohly commented Jun 18, 2022

dims commented Jun 18, 2022

k8s-ci-robot commented Jun 18, 2022

	if k, ok := k.(string); ok {
	// Avoid one allocation when the key is a string, which
	// normally it should be.
	b.WriteString(k)
	} else {
	b.WriteString(fmt.Sprintf("%s", k))
	}

GIT-331: fix shadowing key from the kv pair #332

GIT-331: fix shadowing key from the kv pair #332

Conversation

harshanarayana commented Jun 18, 2022

pohly left a comment

Choose a reason for hiding this comment

pohly commented Jun 18, 2022

dims commented Jun 18, 2022

k8s-ci-robot commented Jun 18, 2022